Test: Hooks Glue Exporter #6721

Isan-Rivkin · 2023-10-08T07:46:45Z

Closes #6686

This PR test AWS Exporters (symlink_exporter and glue_exporter).
The test runs in AWS context only.

Test flow

Symlink Exporter:

Generate CSV Data, lua script with symlink exporter, table for _lakefs_tables and upload.
Generate Action file that will trigger the script.
Wait until the symlinks are created in S3.
Verify that the symlink files on S3 are as expected.

Glue Exporter - Relies on previous step and will create a glue table based on data in that commit:

Generate lua script with glue exporter, lakefs_action to trigger it and upload.
Wait for Table to be created in Glue.
Verify the table created correctly.
Delete the table when done.

arielshaqed

Approving as this is a significant improvement. But note that the condition for running the test is incorrect: we should look at whether we are testing AWS, not whether we are testing S3. Ideally we could fix this now, rather than carry tech debt for whenever we want to test on MinIO somehow.

arielshaqed · 2023-10-18T09:04:04Z

esti/catalog_export_test.go

+	TableSpec           *hiveTableSpec
+}
+
+const glueExportScript = `


Can you document this please? Docs should say it's Lua (I think...), but also explain what {{...}} in it means.

As an alternative, Go has a statically-linked readonly filesystem available. If you used that you could put this in a file, statically link that file into the filesystem, and then the test could read from this file.

Changed all the templates / strings everything to files with go template!

arielshaqed · 2023-10-18T09:04:39Z

esti/catalog_export_test.go

+exporter.export_glue(glue, args.catalog.db_name, args.table_source, args.catalog.table_input, action, {debug=true})
+`
+
+const glueExporterAction = `


Here too. AFAICT this is YAML.

Changed all the templates / strings everything to files with go template!

arielshaqed · 2023-10-18T09:06:34Z

esti/catalog_export_test.go

+
+func setupGlueClient(ctx context.Context, accessKeyID, secretAccessKey string) (*glue.Client, error) {
+	cfg, err := config.LoadDefaultConfig(ctx,
+		config.WithRegion("us-east-1"),


I think we have an envariable with the region.

👍 Made it configurable (its tied to the location of the db)

arielshaqed · 2023-10-18T09:06:51Z

esti/catalog_export_test.go

+	require.Equal(t, http.StatusCreated, commitResp.StatusCode())
+	return commitResp.JSON201
+}
+func genCSVData(cols []string, n int) string {


Suggested change

func genCSVData(cols []string, n int) string {

func genCSVData(cols []string, n int) string {

Please consider using the standard encoding/csv package. Encoding CSVs like this is incredibly brittle.

arielshaqed · 2023-10-18T09:09:26Z

esti/catalog_export_test.go

+func testSymlinkS3Exporter(t *testing.T, ctx context.Context, repo string, tablePaths map[string]string, testData *exportHooksTestData) (string, string) {
+	t.Helper()
+
+	tableYaml, err := yaml.Marshal(&testData.TableSpec)


Could we just store testData.TableSpec as an interface{}, or possibly a map[string]interface{}, and avoid having to write YAML in code, and then deserialize it back into a map?

No can do. Because that struct is not only used for YAML but also as configuration parameters during the test.
Therefore, .I would still have to maintain variables for all of those, why not just map 1 to 1 with the YAML anyway.

arielshaqed · 2023-10-18T09:10:13Z

esti/catalog_export_test.go

+
+	// upload action
+	commit := uploadAndCommitObjects(t, ctx, repo, mainBranch, map[string]string{
+		"_lakefs_actions/animals_symlink.yaml": renderTplAsStr(t, testData, "action", symlinkExporterAction),


Why does this upload need to be separate from the one on l.196?

I don't you're right consolidated everything into one upload, thanks!

arielshaqed · 2023-10-18T09:15:10Z

esti/catalog_export_test.go

+	bo := backoff.NewExponentialBackOff()
+	bo.MaxInterval = 5 * time.Second
+	bo.MaxElapsedTime = 30 * time.Second


Given that ExponentialBackoff is an exported type, why not just

bo := &backoff.ExponentialBackoff{ MaxInterval: 5 * time.Second, MaxElapsedTime: 30 * time.Second, }

?

Because it has other variables inside that New func that we want to use as defaults and are not set when calling backoff.ExponentialBackoff{}

arielshaqed · 2023-10-18T09:15:45Z

esti/catalog_export_test.go

+		})
+		require.NoErrorf(t, err, "listing symlink files in storage: %s", symlinksPrefix)
+		if len(listResp.Contents) == 0 {
+			return fmt.Errorf("no objects found")


Prefer Errors.new perhaps?
But I am not sure that returning this as a specific error gives more information than just returning the empty list -- which will also output the expected results.

i want the retry to keep running if there are no objects found, that's why I'm returning an error.
Per the message itself changed to errors.new

arielshaqed · 2023-10-18T09:29:13Z

esti/catalog_export_test.go

+// Symlinks export: lua script, table in _lakefs_tables, action file, mock table data in CSV form
+// Glue export: lua script, table in _lakefs_tables, action file
+func TestAWSCatalogExport(t *testing.T) {
+	requireBlockstoreType(t, block.BlockstoreTypeS3)


I am not sure that this is the ideal thing to use. We are interested in running on AWS, not in running of S3. Suppose we were running on MinIO: presumably Glue would not work there.

Done! Thanks!

itaiad200 · 2023-10-19T14:54:12Z

Important test! Thank you

Isan-Rivkin added 3 commits October 3, 2023 17:20

lua test

c6575ca

Merge branch 'master' into 6686-lua-export-tests

4ef3af3

initial commit

02ac8ef

Isan-Rivkin added area/testing Improvements or additions to tests export-hooks labels Oct 8, 2023

Isan-Rivkin self-assigned this Oct 8, 2023

Isan-Rivkin changed the title ~~6691 lua glue export test~~ WIP: Test: Hooks Glue Exporter Oct 8, 2023

Isan-Rivkin added 7 commits October 16, 2023 11:44

fix conflict

2b58121

added esti test

7cdcebd

update test info

dd945cf

rerun roles

b816fab

fix require assert table name

fbcb5fa

configure skip if not aws

a092640

fix tests

e6d2f3a

Isan-Rivkin requested a review from arielshaqed October 18, 2023 08:13

Isan-Rivkin added the exclude-changelog PR description should not be included in next release changelog label Oct 18, 2023

arielshaqed approved these changes Oct 18, 2023

View reviewed changes

Isan-Rivkin changed the title ~~WIP: Test: Hooks Glue Exporter~~ Test: Hooks Glue Exporter Oct 19, 2023

Isan-Rivkin added 2 commits October 19, 2023 14:49

fix all review comments

b35911f

prep for merge

14b4ac2

Isan-Rivkin merged commit 7d9feeb into master Oct 19, 2023
29 checks passed

Isan-Rivkin deleted the 6691-lua-glue-export-test branch October 19, 2023 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test: Hooks Glue Exporter #6721

Test: Hooks Glue Exporter #6721

Isan-Rivkin commented Oct 8, 2023 •

edited

Loading

arielshaqed left a comment

arielshaqed Oct 18, 2023

arielshaqed Oct 18, 2023

Isan-Rivkin Oct 19, 2023

arielshaqed Oct 18, 2023

Isan-Rivkin Oct 19, 2023

arielshaqed Oct 18, 2023

Isan-Rivkin Oct 19, 2023

arielshaqed Oct 18, 2023

arielshaqed Oct 18, 2023

Isan-Rivkin Oct 19, 2023

arielshaqed Oct 18, 2023

Isan-Rivkin Oct 19, 2023

arielshaqed Oct 18, 2023

Isan-Rivkin Oct 19, 2023

arielshaqed Oct 18, 2023

Isan-Rivkin Oct 19, 2023

arielshaqed Oct 18, 2023

Isan-Rivkin Oct 19, 2023

arielshaqed Oct 18, 2023

Isan-Rivkin Oct 19, 2023

itaiad200 commented Oct 19, 2023

	func genCSVData(cols []string, n int) string {

	func genCSVData(cols []string, n int) string {

Test: Hooks Glue Exporter #6721

Test: Hooks Glue Exporter #6721

Conversation

Isan-Rivkin commented Oct 8, 2023 • edited Loading

Test flow

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiad200 commented Oct 19, 2023

Isan-Rivkin commented Oct 8, 2023 •

edited

Loading